chore(manifest): add security_group to env manifest by paragbhingre · Pull Request #3749 · aws/copilot-cli

paragbhingre · 2022-07-12T18:18:37Z

This PR fix the issue #3719

Now customers can define security group rules for EnvironmentSecurityGroup (e.g. ingress/egress) in their env manifest

Sample manifest template with security group rules is given below

network:
  vpc:
    security_group:
      ingress:
        IpProtocol: tcp
        FromPort: 80
        ToPort: 80
        CidrIp: 0.0.0.0/0
      egress:
        IpProtocol: tcp
        FromPort: 80
        ToPort: 80
        CidrIp: 0.0.0.0/0

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

bvtujo

couple of dumb questions but this looks great!

bvtujo · 2022-07-13T20:43:25Z

🚀

huanjani · 2022-07-15T16:49:51Z

+		}
+		egress = strings.TrimSpace(string(out))
+	}
+	return &template.SecurityGroupConfig{


So if the Ingress/Egress fields are empty, this will return a *template.SecurityGroupConfig object with nil values for ingress and egress?

Would it be more efficient to return earlier if IsZero, as that's the more common use case?

huanjani · 2022-07-15T16:57:58Z

Looks good!
Are there docs showing users how to specify their rules correctly?
Would you mind changing the example in your PR description to match the changes from your latest commit?
Feel free to remove the label when you're ready!

huanjani

Just a couple small comments!

efekarakus · 2022-07-15T17:08:29Z

Is it possible to hold on to merging this PR until after the release please 🥺

If we can we'd like to halt on env template CFN changes unless it's about the env manifest. Once released, we can merge the PR!

iamhopaul123 · 2022-07-15T17:09:12Z

+{{.SecurityGroupConfig.Ingress | indent 8}}
+{{- end }}
+{{- if and .SecurityGroupConfig .SecurityGroupConfig.Egress }}
+      SecurityGroupEgress:
+{{.SecurityGroupConfig.Egress | indent 8}}


Ideally we don't put yaml.node for them because then it wholly depends on customers to write CFN template which is very similar to what addons should do. As a manifest field, it is usually with some level of abstraction (integrated logic). For example, allow_vpc_ingress and from_cdn

bvtujo · 2022-07-15T18:37:09Z


+// SecurityGroupConfig holds the fields to import security group config
+type SecurityGroupConfig struct {
+	Ingress string


So does the marshaling logic depend on the fact that customers have to specify the exact same keys in the manifest vs in CF?

For example, the manifest config

network: vpc: security_group: ingress: - IpProtocol: tcp FromPort: 0 ToPort: 65535 - IpProtocol: tcp FromPort: 1 ToPort: 6 egress: - IpProtocol: tcp FromPort: 0 ToPort: 65535`

maps to the CloudFormation:

SecurityGroupIngress: - IpProtocol: tcp FromPort: 0 ToPort: 65535 - IpProtocol: tcp FromPort: 1 ToPort: 6 SecurityGroupEgress: - IpProtocol: tcp FromPort: 0 ToPort: 65535

but this means that the customer has to write manifest keys in two separate capitalization styles, which may be confusing. Are there any other places in the manifest that we directly use CF keys versus remapping them to their snake case counterparts (protocol, from_port, and to_port)?

paragbhingre · 2022-07-22T20:34:37Z

To address everyone's concern (including @bvtujo , @iamhopaul123 ) about accepting security_group as yaml.Node or other concern as if we should be allowing customers to specify CFN template inside manifest, I would like to mention that as per my discussion with @efekarakus we are going to implement security_group rules with restricted fields only. That means we will be not using yaml.Node neither we allow customers to define CFN fields inside manifest. The expected input from the customer would be,

security_groups:
 ingress:
    - cidr:
      from_port:
      to_port:
      ip_protocol:

We will target just CIDR for now which will solve some use cases such as mentioned here

rclinard-amzn · 2022-07-22T21:21:41Z

It looks like this doesn't pass CI, likely due to a broken merge?

This reverts commit 5de9487.

Reverts #3749: chore(manifest): add security_group to env manifest Reverting this PR as we will be implementing new solution for this issue.

This PR fix the issue #3719 Related to [#3749](#3749) Now customers can define security group rules for EnvironmentSecurityGroup (e.g. ingress/egress) in their env manifest Sample manifest template with security group rules is given below ``` network: vpc: security_group: ingress: - ip_protocol: tcp from_port: 80 to_port: 80 cidr: 0.0.0.0/0 egress: - ip_protocol: tcp from_port: 80 to_port: 80 cidr: 0.0.0.0/0 ```

chore(manifest): add security_group to env manifest

7913f6a

paragbhingre requested a review from a team as a code owner July 12, 2022 18:18

paragbhingre requested review from efekarakus and removed request for a team July 12, 2022 18:18

amazon-ecs-cli-v2-pr-manager requested a review from Lou1415926 July 12, 2022 18:18

bvtujo reviewed Jul 12, 2022

View reviewed changes

Comment thread internal/pkg/deploy/cloudformation/stack/env.go Outdated

Comment thread internal/pkg/deploy/cloudformation/stack/env.go Outdated

Comment thread internal/pkg/manifest/env.go Outdated

paragbhingre added 5 commits July 12, 2022 13:14

resolve merge conflicts

d7a218d

Merge branch 'mainline' into EnvironmentSecurityGroup

c42f8fa

resolve static check error

6869f69

Address feedback

b0cfe54

nit

32a21f3

bvtujo approved these changes Jul 13, 2022

View reviewed changes

huanjani reviewed Jul 13, 2022

View reviewed changes

Comment thread internal/pkg/deploy/cloudformation/stack/env_integration_test.go Outdated

paragbhingre added 5 commits July 14, 2022 11:18

typo

f3bdd6f

Merge branch 'mainline' into EnvironmentSecurityGroup

95a5b3d

fix integ test

ade35e5

Merge branch 'mainline' into EnvironmentSecurityGroup

78dfcc1

added removed part during merge conflicts

5170593

paragbhingre added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 15, 2022

remodified the code to incorporate multiple ingress and egress rules

7572d72

huanjani reviewed Jul 15, 2022

View reviewed changes

huanjani approved these changes Jul 15, 2022

View reviewed changes

iamhopaul123 reviewed Jul 15, 2022

View reviewed changes

bvtujo reviewed Jul 15, 2022

View reviewed changes

efekarakus removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 19, 2022

Merge branch 'mainline' into EnvironmentSecurityGroup

2ea7767

fix merge conflict and integ test

961b91c

mergify Bot merged commit 5de9487 into aws:mainline Jul 22, 2022

paragbhingre added a commit that referenced this pull request Jul 22, 2022

Revert "chore(manifest): add security_group to env manifest (#3749)"

b438743

This reverts commit 5de9487.

paragbhingre mentioned this pull request Jul 22, 2022

chore: revert #3749 (5de9487) #3803

Merged

mergify Bot pushed a commit that referenced this pull request Jul 22, 2022

chore: revert #3749 (5de9487) (#3803)

a26fc1b

Reverts #3749: chore(manifest): add security_group to env manifest Reverting this PR as we will be implementing new solution for this issue.

paragbhingre mentioned this pull request Jul 25, 2022

feat: configure environment security group from env manifest #3810

Merged

paragbhingre deleted the EnvironmentSecurityGroup branch January 26, 2023 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(manifest): add security_group to env manifest#3749

chore(manifest): add security_group to env manifest#3749
mergify[bot] merged 14 commits intoaws:mainlinefrom
paragbhingre:EnvironmentSecurityGroup

paragbhingre commented Jul 12, 2022 •

edited

Loading

Uh oh!

bvtujo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bvtujo commented Jul 13, 2022

Uh oh!

Uh oh!

huanjani Jul 15, 2022

Uh oh!

huanjani commented Jul 15, 2022

Uh oh!

huanjani left a comment

Uh oh!

efekarakus commented Jul 15, 2022

Uh oh!

iamhopaul123 Jul 15, 2022

Uh oh!

bvtujo Jul 15, 2022

Uh oh!

paragbhingre commented Jul 22, 2022

Uh oh!

rclinard-amzn commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

paragbhingre commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvtujo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bvtujo commented Jul 13, 2022

Uh oh!

Uh oh!

huanjani Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

huanjani commented Jul 15, 2022

Uh oh!

huanjani left a comment

Choose a reason for hiding this comment

Uh oh!

efekarakus commented Jul 15, 2022

Uh oh!

iamhopaul123 Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

bvtujo Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

paragbhingre commented Jul 22, 2022

Uh oh!

rclinard-amzn commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

paragbhingre commented Jul 12, 2022 •

edited

Loading